gh-130168: pyrepl can messup and poll is not thread safe #138617

yihong0618 · 2025-09-07T11:52:52Z

This fix issue 130168
but for this one it contains two issues:

the first one is for poll here is not thread safe
more can check. ##53111

and the second one is for the prepare and restore the error message will mess up.

and for the default pyrepl >= 3.13 the input is wrote in python different from the old one

this patch make the error same which is can't re-enter readline like python3.12

before this patch:

after this patch:
which not mess up anything

cc @gaogaotiantian can you help to check?

Thank you very much.

Issue: pyrepl messed up terminal if a signal handler expects stdin #130168

Signed-off-by: yihong0618 <[email protected]>

Lib/_pyrepl/unix_console.py

Lib/test/test_pyrepl/test_unix_console.py

Misc/NEWS.d/next/Library/2025-09-07-19-47-35.gh-issue-130168.qvtlOZ.rst

Co-authored-by: Bénédikt Tran <[email protected]>

Signed-off-by: yihong0618 <[email protected]>

picnixz · 2025-09-07T12:25:10Z

Hum, now tests fail but I don't know why. Is there another place where we use the internal state?

yihong0618 · 2025-09-07T12:27:10Z

Hum, now tests fail but I don't know why. Is there another place where we use the internal state?

same like #124030

picnixz · 2025-09-07T12:30:00Z

Yeah but https://github.com/python/cpython/actions/runs/17528155338/job/49781429653?pr=138617 worked and it was before my suggestions. Ok, remove the addCleanup part, this may be the culprit and do it normally (that is as you did before)

Signed-off-by: yihong0618 <[email protected]>

yihong0618 · 2025-09-07T12:34:10Z

Yeah but https://github.com/python/cpython/actions/runs/17528155338/job/49781429653?pr=138617 worked and it was before my suggestions. Ok, remove the addCleanup part, this may be the culprit and do it normally (that is as you did before)

Done will check the test

picnixz · 2025-09-07T12:51:39Z

Mmh, so apparently the issue is indeed with the cleanup. Though I don't understand why.... because cleanups are called in LIFO.

picnixz · 2025-09-07T12:53:51Z

When I meant "remove it", I meant remove the addCleanup, but keep console._polling_thread = None; console.restore() at the end of the test function.

Signed-off-by: yihong0618 <[email protected]>

yihong0618 · 2025-09-07T12:59:06Z

When I meant "remove it", I meant remove the addCleanup, but keep console._polling_thread = None; console.restore() at the end of the test function.

sorry for forget that, now move it back

yihong0618 · 2025-09-07T14:00:59Z

Mmh, so apparently the issue is indeed with the cleanup. Though I don't understand why.... because cleanups are called in LIFO.

do we need to open an issue to track this?

yihong0618 · 2025-09-08T05:51:58Z

This patch also make #129614 the same behavior as 3.12

picnixz · 2025-09-08T07:42:50Z

do we need to open an issue to track this?

Maybe, but I'll need to check with a smaller reproducer.

yihong0618 · 2025-09-15T04:06:29Z

Mmh, so apparently the issue is indeed with the cleanup. Though I don't understand why.... because cleanups are called in LIFO.

@picnixz after some dig found why here

the issue is that we mock the tcgetattr the arg in the patch

@patch(
    "termios.tcgetattr",
    lambda _: [
        27394,
        3,
        19200,
        536872399,
        38400,
        38400,
        [
            b"\x04",
            b"\xff",
            b"\xff",
            b"\x7f",
            b"\x17",
            b"\x15",
            b"\x12",
            b"\x00",
            b"\x03",
            b"\x1c",
            b"\x1a",
            b"\x19",
            b"\x11",
            b"\x13",
            b"\x16",
            b"\x0f",
            b"\x01",
            b"\x00",
            b"\x14",
            b"\x00",
        ],
    ],
)
@patch("termios.tcsetattr", lambda a, b, c: None)

it is fine but in some system like ubuntu arm 24
the length of it is 32

but

this length is 20

    [
        b"\x04",
        b"\xff",
        b"\xff",
        b"\x7f",
        b"\x17",
        b"\x15",
        b"\x12",
        b"\x00",
        b"\x03",
        b"\x1c",
        b"\x1a",
        b"\x19",
        b"\x11",
        b"\x13",
        b"\x16",
        b"\x0f",
        b"\x01",
        b"\x00",
        b"\x14",
        b"\x00",
    ],

you can use this script to check

import sys
import termios

sys.path.insert(0, "./Lib")

from _pyrepl.fancy_termios import tcgetattr, tcsetattr


def main():
    try:
        state = tcgetattr(0)
        print("Real tcgetattr works, cc length:", len(state.cc))

        tcsetattr(0, termios.TCSADRAIN, state)
        print("Real tcsetattr works")
    except Exception as e:
        print("Error:", e)


if __name__ == "__main__":
    main()

if we use self.addclean ... it calls the patch and the length is different

[hyi@rocky cpython]$ uname -a
Linux rocky 6.15.11-orbstack-00539-g9885ebd8e3f4 #1 SMP PREEMPT Fri Aug 22 08:24:56 UTC 2025 aarch64 aarch64 aarch64 GNU/Linux
[hyi@rocky cpython]$ ./python test.py 
Real tcgetattr works, cc length: 32
Real tcsetattr works

➜  cpython git:(hy/close_new_repl_error_mess) ✗ uname -a
Darwin hyideMacBook-Pro.local 24.5.0 Darwin Kernel Version 24.5.0: Tue Apr 22 19:54:33 PDT 2025; root:xnu-11417.121.6~2/RELEASE_ARM64_T8122 arm64
➜  cpython git:(hy/close_new_repl_error_mess) ✗ ./python.exe test.py 
Real tcgetattr works, cc length: 20
Real tcsetattr works

so I think we add console.restore() in the last is fine

yihong0618 · 2025-09-17T01:45:23Z

merge and fix the conflict

yihong0618 · 2025-09-17T01:54:35Z

interesting fail will try to figure out why

fixed

Signed-off-by: yihong0618 <[email protected]>

chris-eibl · 2025-09-23T10:43:59Z

I gave this PR a try on native Ubuntu 24.04.3 LTS, and it now shows RuntimeError: can't re-enter readline, but the output is still messed up.

Details

chris-eibl · 2025-09-23T10:45:18Z

it now shows RuntimeError: can't re-enter readline

I understand this is to be in sync what the old REPL showed, but isn't that misleading, because here readline isn't involved at all?

picnixz · 2025-09-23T10:49:39Z

I understand this is to be in sync what the old REPL showed, but isn't that misleading, because here readline isn't involved at all?

It is, through reader.readline(). But it's not the readline you think of. But we can think of a better message as it can be confusing indeed.

I gave this PR a try on native Ubuntu 24.04.3 LTS, and it now shows RuntimeError: can't re-enter readline, but the output is still messed up.

Mmh, then the fix should be different probably. The OP's output seems to be on darwin which is Unix-based as well. Note that unix_console defines a local poll class that is re-implemented in case select.poll is unavailable.

In addition, on Apple terminals, line wrap is disabled so maybe something is different here as well?

chris-eibl · 2025-09-23T10:54:24Z

Just FTR, confirming that Windows isn't affected (since as said like on Linux no readline is involved and the WaitForSingleObject seems to be re-entrant.

Legacy console:

Virtual terminal:

The prompt is overridden in this case, but this will be solved by #138732.

yihong0618 · 2025-09-23T10:55:02Z

I gave this PR a try on native Ubuntu 24.04.3 LTS, and it now shows RuntimeError: can't re-enter readline, but the output is still messed up.

Details

Interesting will test it

chris-eibl · 2025-09-23T11:01:35Z

It is, through reader.readline(). But it's not the readline you think of. But we can think of a better message as it can be confusing indeed.

Oh, I know :) reader.readline() shows in the traceback.

I was referring to

this patch make the error same which is can't re-enter readline like python3.12

Why not just fail with "concurrent poll" - why mimic the 3.12 error message here?
I mean it is not totally wrong, but might be confusing ...

yihong0618 · 2025-09-23T11:06:52Z

It is, through reader.readline(). But it's not the readline you think of. But we can think of a better message as it can be confusing indeed.

Oh, I know :) reader.readline() shows in the traceback.

I was referring to

this patch make the error same which is can't re-enter readline like python3.12

Why not just fail with "concurrent poll" - why mimic the 3.12 error message here?

I mean it is not totally wrong, but might be confusing ...

sorry for the confusion next time will try to use LLM or something else to make a better desc.
sorry for that

yihong0618 · 2025-09-25T10:02:17Z

I gave this PR a try on native Ubuntu 24.04.3 LTS, and it now shows RuntimeError: can't re-enter readline, but the output is still messed up.

Details

yes it is mess up
merge main break the fix ....
will figure out why

Signed-off-by: yihong0618 <[email protected]>

yihong0618 · 2025-09-25T13:20:36Z

@chris-eibl now fixed the mess up can you help to check?

chris-eibl · 2025-09-25T13:50:32Z

Works now for me on native Ubuntu 24.04.3 LTS.

yihong0618 · 2025-09-25T13:53:51Z

Works now for me on native Ubuntu 24.04.3 LTS.

thank you for confirm the reason is that merge conflict~

chris-eibl · 2025-09-25T13:55:37Z

Still unsure about the RuntimeError: can't re-enter readline.

IMHO the PR would be simpler without trying to mimic it, but that's the the pyrepl maintainers decision, anyway.

yihong0618 · 2025-09-25T14:05:41Z

Still unsure about the RuntimeError: can't re-enter readline.

IMHO the PR would be simpler without trying to mimic it, but that's the the pyrepl maintainers decision, anyway.

the reason I choose that is make the runtime error message as basic repl

picnixz · 2025-09-29T09:54:27Z

Lib/_pyrepl/unix_console.py

+            # Forbid re-entrant calls and use the old REPL error message.
+            raise RuntimeError("can't re-enter readline")


Yeah, maybe we can change the message. I don't have a suggestion now but I'd like REPL maintainers to first check this PR.

fix: pyrepl can messup and poll is not thread safe

0609120

Signed-off-by: yihong0618 <[email protected]>

yihong0618 requested review from pablogsal, lysnikolaou and ambv as code owners September 7, 2025 11:52

bedevere-app bot added the awaiting review label Sep 7, 2025

yihong0618 changed the title ~~fix: pyrepl can messup and poll is not thread safe~~ gh-130168: pyrepl can messup and poll is not thread safe Sep 7, 2025

bedevere-app bot mentioned this pull request Sep 7, 2025

pyrepl messed up terminal if a signal handler expects stdin #130168

Open

fix: mypy type check

ffc77f2

Signed-off-by: yihong0618 <[email protected]>

picnixz reviewed Sep 7, 2025

View reviewed changes

picnixz requested a review from gaogaotiantian September 7, 2025 12:09

yihong0618 and others added 2 commits September 7, 2025 20:12

fix: apply suggestions from code review

8fa9c19

Co-authored-by: Bénédikt Tran <[email protected]>

fix: __svtermstate default None

78d7416

Signed-off-by: yihong0618 <[email protected]>

yihong0618 requested a review from picnixz September 7, 2025 12:16

fix: drop cleanup part for new test

ca27ae4

Signed-off-by: yihong0618 <[email protected]>

fix: forget to add clean up back

b194f8f

Signed-off-by: yihong0618 <[email protected]>

yihong0618 mentioned this pull request Sep 12, 2025

python -m asyncio and breakpoint() will fovever error loop and mess up the terminal #138827

Open

StanFromIreland linked an issue Sep 12, 2025 that may be closed by this pull request

python -m asyncio and breakpoint() will fovever error loop and mess up the terminal #138827

Open

Merge branch 'main' into hy/close_new_repl_error_mess

e43fc60

fix: tests failed by wrong conflict fix

5d83648

Signed-off-by: yihong0618 <[email protected]>

chris-eibl added the topic-repl Related to the interactive shell label Sep 17, 2025

Merge branch 'main' into hy/close_new_repl_error_mess

b3a41eb

fix: merge regress

17877b6

Signed-off-by: yihong0618 <[email protected]>

picnixz reviewed Sep 29, 2025

View reviewed changes

		# Forbid re-entrant calls and use the old REPL error message.
		raise RuntimeError("can't re-enter readline")

Uh oh!

gh-130168: pyrepl can messup and poll is not thread safe #138617

Are you sure you want to change the base?

gh-130168: pyrepl can messup and poll is not thread safe #138617

Conversation

yihong0618 commented Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

picnixz commented Sep 7, 2025

Uh oh!

yihong0618 commented Sep 7, 2025

Uh oh!

picnixz commented Sep 7, 2025

Uh oh!

yihong0618 commented Sep 7, 2025

Uh oh!

picnixz commented Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

picnixz commented Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yihong0618 commented Sep 7, 2025

Uh oh!

yihong0618 commented Sep 7, 2025

Uh oh!

yihong0618 commented Sep 8, 2025

Uh oh!

picnixz commented Sep 8, 2025

Uh oh!

yihong0618 commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yihong0618 commented Sep 17, 2025

Uh oh!

yihong0618 commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chris-eibl commented Sep 23, 2025

Uh oh!

chris-eibl commented Sep 23, 2025

Uh oh!

picnixz commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chris-eibl commented Sep 23, 2025

Uh oh!

yihong0618 commented Sep 23, 2025

Uh oh!

chris-eibl commented Sep 23, 2025

Uh oh!

yihong0618 commented Sep 23, 2025

Uh oh!

yihong0618 commented Sep 25, 2025

Uh oh!

yihong0618 commented Sep 25, 2025

Uh oh!

chris-eibl commented Sep 25, 2025

Uh oh!

yihong0618 commented Sep 25, 2025

Uh oh!

chris-eibl commented Sep 25, 2025

Uh oh!

yihong0618 commented Sep 25, 2025

Uh oh!

picnixz Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yihong0618 commented Sep 7, 2025 •

edited

Loading

picnixz commented Sep 7, 2025 •

edited

Loading

picnixz commented Sep 7, 2025 •

edited

Loading

yihong0618 commented Sep 15, 2025 •

edited

Loading

yihong0618 commented Sep 17, 2025 •

edited

Loading

picnixz commented Sep 23, 2025 •

edited

Loading